Skip to content

Support new file/format model shape for local_server.kv_store and secret_store (as well as metadata in kv_store)#1446

Merged
harmony7 merged 5 commits intomainfrom
kats/fastly_toml_local_server
Apr 4, 2025
Merged

Support new file/format model shape for local_server.kv_store and secret_store (as well as metadata in kv_store)#1446
harmony7 merged 5 commits intomainfrom
kats/fastly_toml_local_server

Conversation

@harmony7
Copy link
Copy Markdown
Member

@harmony7 harmony7 commented Mar 31, 2025 •
edited
Loading

Viceroy's KV Store can now be configured using external JSON files specified using fastly.toml (fastly/Viceroy#365 and fastly/Viceroy#428).

However, Fastly CLI's fastly.toml validation would choke and it was not possible to use the new syntax.

✗ Verifying fastly.toml

ERROR: (0, 0): Can't convert file = "./foo.json"
format = "json"
(*toml.Tree) to a tree.

This PR updates the TOML marshaling/unmarshaling mechanism to enable the new syntax in fastly.toml.

Fixes #1319

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

User Impact

Are there any considerations that need to be addressed for release?

This should be backwards compatible.

@harmony7 harmony7 requested a review from a team as a code owner March 31, 2025 07:07
@harmony7 harmony7 force-pushed the kats/fastly_toml_local_server branch 2 times, most recently from b823833 to 5d46115 Compare March 31, 2025 07:10
@harmony7
Copy link
Copy Markdown
Member Author

harmony7 commented Mar 31, 2025

Working with Marshal and UnmarshalJSON have been pretty crazy, there may be cleaner ways to do this. I'm open to any suggestions.

@harmony7 harmony7 force-pushed the kats/fastly_toml_local_server branch from 5d46115 to 312da69 Compare March 31, 2025 11:34
@harmony7 harmony7 force-pushed the kats/fastly_toml_local_server branch from 312da69 to 7f58837 Compare March 31, 2025 11:48
@kpfleming kpfleming removed the request for review from Integralist March 31, 2025 13:41
@kpfleming
Copy link
Copy Markdown
Member

kpfleming commented Mar 31, 2025

No need to request a review from @Integralist for this, the DevTools team can handle it now.

philippschulte
philippschulte requested changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found only some nit picks otherwise this looks good to me!

With the following config:

philipp@philipp-XPS-15-9500:~/src/fastly/cli/ps$ cat fastly.toml 
# This file describes a Fastly Compute package. To learn more visit:
# https://www.fastly.com/documentation/reference/compute/fastly-toml

authors = ["ps@gmail.com"]
cloned_from = "https://github.com/fastly/compute-starter-kit-rust-default"
description = "ps"
language = "rust"
manifest_version = 3
name = "ps"
service_id = ""

[local_server]
kv_stores.store_name = {file = "entries.json", format = "json"}

[scripts]
  build = "cargo build --profile release"

everything builds:

philipp@philipp-XPS-15-9500:~/src/fastly/cli/ps$ ./fastly compute build
✓ Verifying fastly.toml
✓ Identifying package name
✓ Identifying toolchain
✓ Running [scripts.build]
✓ Creating package archive

SUCCESS: Built package (pkg/ps.tar.gz)

Comment thread pkg/manifest/file.go Outdated
Comment thread pkg/manifest/file.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go
Comment thread pkg/manifest/testdata/fastly-viceroy-update.toml
@harmony7
Copy link
Copy Markdown
Member Author

harmony7 commented Apr 2, 2025

Thanks for your review.
I have applied your suggestions, and seen that the tests pass again.

Please review once more =)

@harmony7 harmony7 requested a review from philippschulte April 2, 2025 06:35
philippschulte
philippschulte requested changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. I believe we don't need those wrapper types in the test suite. Please have a look and let me know. Thanks.

Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
Comment thread pkg/manifest/local_server_test.go Outdated
kpfleming
kpfleming approved these changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions, only one of which is potentially blocking.

Comment thread pkg/manifest/file.go
Comment thread pkg/manifest/local_server.go
@harmony7 harmony7 changed the title Support file/format for kv_store and secret_store Support new file/format model shape for local_server.kv_store and secret_store (as well as metadata in kv_store) Apr 2, 2025
@harmony7 harmony7 force-pushed the kats/fastly_toml_local_server branch from 85be089 to 9549508 Compare April 2, 2025 23:17
@harmony7
Copy link
Copy Markdown
Member Author

harmony7 commented Apr 2, 2025

Thanks for your comments, @philippschulte
I've pushed a commit to address the test, please take a look.

@harmony7 harmony7 requested a review from philippschulte April 2, 2025 23:26
@harmony7
Copy link
Copy Markdown
Member Author

harmony7 commented Apr 2, 2025

Thanks for your comments, @kpfleming
I tried adding metadata to fastly-viceroy-update.toml and saw that the test broke.
So I've pushed a commit that adds support for Metadata and fix the test case.

@harmony7 harmony7 requested a review from kpfleming April 2, 2025 23:28
philippschulte
philippschulte approved these changes Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for addressing all of my comments.

kpfleming
kpfleming approved these changes Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Once this is merged we'll need to update the fastly.toml docs too.

@joeshaw
Copy link
Copy Markdown
Member

joeshaw commented Apr 3, 2025

Thank you for tackling this! 👍

@harmony7
Copy link
Copy Markdown
Member Author

harmony7 commented Apr 4, 2025

Thanks everyone!

Once this is merged we'll need to update the fastly.toml docs too.

Agreed, I'll work on that too.

@harmony7 harmony7 enabled auto-merge (squash) April 4, 2025 02:39
@harmony7 harmony7 merged commit cf4af94 into main Apr 4, 2025
11 checks passed
@harmony7 harmony7 deleted the kats/fastly_toml_local_server branch April 4, 2025 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kpfleming kpfleming kpfleming approved these changes

@philippschulte philippschulte philippschulte approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CLI doesn't handle new KV store (and upcoming secret store) local server config

4 participants

@harmony7 @kpfleming @joeshaw @philippschulte